Skip to content

Stop putting Assumes in MIR for every enum-as cast #138297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Stop putting Assumes in MIR for every enum-as cast #138297

wants to merge 2 commits into from

Conversation

@scottmcm
Copy link
Member

@scottmcm scottmcm commented Mar 10, 2025 •
edited
Loading

Now that we have range attributes on parameters -- so an Ordering passed by-value is already known to be in -1, 0, +1, for example -- these are much less useful than they used to be.

Having all these extra instructions in the MIR means more instructions for MIR passes (including check things like borrowck) and make them look like worse inlining candidates, even though I can't think of a time it'd be bad to inline an as cast on an enum -- after all, if the cast is allowed at all it's because reading the tag does literally nothing in LLVM.

It made sense at the time, but now we have Transmute in MIR, and backends handle appropriate assumeing for that, because as casts can only add assumes for concrete types so any generic cases need to be handled by the backend anyway.

And, TBH, I like to be able to just say "no, don't transmute your field-less enum to an integer -- the as cast produces the identical MIR" if anyone asks, so that they're never tempted to do the unsafe thing by hand.

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2025

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

scottmcm
scottmcm commented Mar 10, 2025
View reviewed changes
tests/mir-opt/building/enum_cast.unreachable.built.after.mir
Comment on lines +16 to +17
assume(const false);
_2 = const 0_u16 as u16 (IntToInt);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why people would be as-casting uninhabited enums, but apparently it's supported 🤷

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@scottmcm
Copy link
Member Author

Oh. Well, I'm glad I added that assert I expected to be impossible 🙃

(I wish we were better about ICE follow-up tests getting a test in the tests for the place that fixed it, rather than everything just getting a UI test that the code now works.)

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
tests/fail/function_calls/exported_symbol_bad_unwind2.rs (revision `definition`) ... ok
tests/fail/function_calls/exported_symbol_bad_unwind2.rs (revision `both`) ... ok

FAILED TEST: tests/fail/validity/invalid_enum_cast.rs
command: MIRI_ENV_VAR_TEST="0" MIRI_TEMP="/tmp/miri-uitest-rd4JWt" RUST_BACKTRACE="1" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/miri" "--error-format=json" "--sysroot=/checkout/obj/build/x86_64-unknown-linux-gnu/miri-sysroot" "-Dwarnings" "-Dunused" "-Ainternal_features" "-Zui-testing" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/miri_ui/0/tests/fail/validity" "tests/fail/validity/invalid_enum_cast.rs" "-Zmiri-disable-alignment-check" "-Zmiri-disable-stacked-borrows" "-Zmiri-disable-validation" "--edition" "2021"

error: test got exit status: 0, but expected 1
 = note: compilation succeeded, but was expected to fail

error: actual output differed from expected
---
-   = note: inside `cast` at tests/fail/validity/invalid_enum_cast.rs:LL:CC
-note: inside `main`
-  --> tests/fail/validity/invalid_enum_cast.rs:LL:CC
-   |
-LL |     cast(&v as *const u32 as *const E);
-   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
-
-error: aborting due to 1 previous error
-


error: `enum value has invalid tag` not found in diagnostics on line 14
##[error]  --> tests/fail/validity/invalid_enum_cast.rs:14:43
   |
14 |         let _val = *ptr as u32; //~ERROR: enum value has invalid tag
   |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected because of this pattern
   |

FAILURES:
    tests/fail/validity/invalid_enum_cast.rs
---

Location:
   /cargo/registry/src/index.crates.io-1949cf8c6b5b557f/ui_test-0.28.0/src/lib.rs:369

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.
error: test failed, to rerun pass `--test ui`

Caused by:
  process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/x86_64-unknown-linux-gnu/release/deps/ui-9d7bd8a6b4242d84 --quiet` (exit status: 1)
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:04:39
  local time: Mon Mar 10 06:09:36 UTC 2025
  network time: Mon, 10 Mar 2025 06:09:36 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@scottmcm scottmcm marked this pull request as draft March 15, 2025 04:11
@bors
Copy link
Collaborator

bors commented Mar 15, 2025

☔ The latest upstream changes (presumably #138464) made this pull request unmergeable. Please resolve the merge conflicts.

@scottmcm
Copy link
Member Author

I now think this is the wrong way to do this.

@scottmcm scottmcm closed this Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@oli-obk oli-obk

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@scottmcm @rustbot @rust-log-analyzer @bors @oli-obk